Skip to content

fix: RemoveEmptyHolding / authorizeMPToken Delete MPToken With Active Escrow (sfLockedAmount > 0)#6635

Open
Kassaking7 wants to merge 6 commits intoXRPLF:developfrom
Kassaking7:DEFI-614
Open

fix: RemoveEmptyHolding / authorizeMPToken Delete MPToken With Active Escrow (sfLockedAmount > 0)#6635
Kassaking7 wants to merge 6 commits intoXRPLF:developfrom
Kassaking7:DEFI-614

Conversation

@Kassaking7
Copy link
Copy Markdown

The removeEmptyHolding function in src/libxrpl/ledger/View.cpp can delete an MPToken ledger object even when it has a non-zero sfLockedAmount (i.e., funds are currently held in escrow). This orphans the escrowed funds: the Escrow ledger object still exists, but the MPToken it references has been removed. Any subsequent attempt to finish or cancel the escrow fails, resulting in permanent loss of the escrowed tokens.

High Level Overview of Change

Context of Change

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Copy link
Copy Markdown
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs to be amendment-gated, since it has already been released with Supported::yes

Copy link
Copy Markdown
Contributor

@PeterChen13579 PeterChen13579 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job on your first PR! Small changes requested to make the tests more readable

@Kassaking7
Copy link
Copy Markdown
Author

Do we have any other fix needed. Maybe we can batch those fixes with one amendment-gate?

@kennyzlei
Copy link
Copy Markdown
Collaborator

Do we have any other fix needed. Maybe we can batch those fixes with one amendment-gate?

@Kassaking7 it is likely we may have one amendment containing other fixes too, @bthomee any suggestions for the fix amendment name here?

@mvadari
Copy link
Copy Markdown
Collaborator

mvadari commented Mar 24, 2026

/ai-review

@xrplf-ai-reviewer
Copy link
Copy Markdown
Contributor

⚠️ Review Error — The automated review failed to complete. Please re-trigger with /ai-review.

Error: Failed to post review to GitHub after 3 attempts: Server error '500 Internal Server Error' for url 'https://api.github.com/repos/XRPLF/rippled/pulls/6635/reviews'

Review by Claude Opus 4.6 · Prompt: v12

@bthomee
Copy link
Copy Markdown
Collaborator

bthomee commented Mar 24, 2026

Do we have any other fix needed. Maybe we can batch those fixes with one amendment-gate?

@Kassaking7 it is likely we may have one amendment containing other fixes too, @bthomee any suggestions for the fix amendment name here?

We don't have a name for that amendment yet, but I'll add this PR to my local release tracker sheet so when the time comes we can ask @Kassaking7 to update this PR.

In the meantime, @Kassaking7, please guard the changes by a new fix amendment with a suitable name, and then later we can swap out the amendment by the collective fix amendment before merging.

Name suggestion for now:

XRPL_FIX    (MPTLockedAmount, Supported::no, VoteBehavior::DefaultNo)

@mvadari mvadari added the AI Triage Bugs and fixes that have been triaged via AI initiatives label Mar 24, 2026
@bthomee
Copy link
Copy Markdown
Collaborator

bthomee commented Mar 24, 2026

/ai-review

Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues.

Review by Claude Opus 4.6 · Prompt: V12

@Kassaking7 Kassaking7 changed the title fix: removeEmptyHolding / authorizeMPToken Delete MPToken With Active Escrow (sfLockedAmount > 0) fix: RemoveEmptyHolding / authorizeMPToken Delete MPToken With Active Escrow (sfLockedAmount > 0) Mar 25, 2026
Copy link
Copy Markdown
Contributor

@PeterChen13579 PeterChen13579 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions
Copy link
Copy Markdown

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@bthomee
Copy link
Copy Markdown
Collaborator

bthomee commented Mar 26, 2026

@Kassaking7 we merged the change that introduces the fix amendment, named fixSecurity_1_3_1 for now. Would you be able to update your PR to use that amendment instead?

@github-actions
Copy link
Copy Markdown

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Triage Bugs and fixes that have been triaged via AI initiatives

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants